buglwIP - A Lightweight TCP/IP stack - Bugs: bug #21107, LwIP doesn't reset the TTL in ICMP...

 
 

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

bug #21107: LwIP doesn't reset the TTL in ICMP Echo Replies

Submitter:  Tom Evans <tom_evans>
Submitted:  Tue 18 Sep 2007 04:46:51 AM UTC
   
 
Category:  IPv4 Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Wed 19 Sep 2007 08:36:58 AM UTC, comment #12: 

Thanks for testing.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 19 Sep 2007 07:09:27 AM UTC, comment #11: 

It build and fix the problem. I attach a capture, with my ICMP_TTL=4, and it's what I got in the capture...

(file #13990)

Frédéric Bernon <fbernon>
Group Member
Wed 19 Sep 2007 06:13:46 AM UTC, comment #10: 

Fixed by including

>> /* Set the correct TTL and recalculate
>> the header checksum. */
>> IPH_TTL_SET(iphdr, ICMP_TTL);
>> IPH_CHKSUM_SET(iphdr, 0);
>> #if CHECKSUM_GEN_IP
>> IPH_CHKSUM_SET(iphdr, inet_chksum(iphdr, IP_HLEN));
>> #endif /* CHECKSUM_GEN_IP */


(hope it compiles as I can't currently check it :( )

Simon Goldschmidt <goldsimon>
Group administrator
Wed 19 Sep 2007 06:01:21 AM UTC, comment #9: 


> Clarity over "Premature optimisation" every time.


For the IP checksum (which is rather fast), that might be true, OK. Especially the '#if CHECKSUM_GEN_IP' is something that might have gone wrong before, too.

On the other hand, if we have the correct code to adjust the ICMP checksum, why wouldn't it work for IP, too?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 18 Sep 2007 11:48:15 PM UTC, comment #8: 

Clarity over "Premature optimisation" every time.

I've been involved in another IP stack that
"adjusted the IP Header Checksum" when acting as a router
instead of recalculating it. When using Telnet through it,
after a few HOURS it would stop working for a while. It
stopped for 256 packets out of every 65536, which is a
dead giveaway.

The IP Sequence number sequential increase made the checksum
roll through the zone where the buggy adjustment got it wrong
and didn't handle the end-around-carries quite properly.

I "fixed" my LwIP code and then it didn't work as I didn't
realise that the code in icmp_input() adjusts the ICMP
Checksum, and not the IP checksum. The former has to be
adjusted as the Request has changed to a Reply. The latter
only has to be recalculated if the TTL is changed.

I've put the following extra code in icmp.c and the tracert
and ping now works:

>     /* adjust the checksum */
>     if (iecho->chksum >= htons(0xffff - (ICMP_ECHO << 8))) {
>       iecho->chksum += htons(ICMP_ECHO << 8) + 1;
>     } else {
>       iecho->chksum += htons(ICMP_ECHO << 8);
>     }
>
>>     /* Set the correct TTL and recalculate
>>        the header checksum. */
>>     IPH_TTL_SET(iphdr, ICMP_TTL);
>>     IPH_CHKSUM_SET(iphdr, 0);
>> #if CHECKSUM_GEN_IP
>>     IPH_CHKSUM_SET(iphdr, inet_chksum(iphdr, IP_HLEN));
>> #endif  /* CHECKSUM_GEN_IP */
>
>     ICMP_STATS_INC(icmp.xmit);
>     /* increase number of messages attempted to send */




Tom Evans <tom_evans>
Tue 18 Sep 2007 10:02:42 AM UTC, comment #7: 


>The icmp echo change is already done, so in addition to your suggested patch, we simply have to substract the old ttl and add the new ttl to the checksum and that should be it.


I'm agree, I just do this remark because it give a code less readable than calling :

IPH_CHKSUM_SET(iphdr, 0);
#if CHECKSUM_GEN_IP
    IPH_CHKSUM_SET(iphdr, inet_chksum(iphdr, IP_HLEN));
#endif

Ok, it's slower (except if your MAC do any offload computing), but I don't think that any device receive "lot" of ICMP echos.

That's why readability & footprint (yes, a very little bit) could be choosen here. But it's just a remark of course, so feel free to change it...



Frédéric Bernon <fbernon>
Group Member
Tue 18 Sep 2007 09:33:32 AM UTC, comment #6: 


>> But the checksum can still be adjusted (which is much faster),
>> instead of being completely recalculated!


> Yes, but in this case, the adjustment is more complex (icmp echo
> change, icmp checksum, and ip ttl)?


The icmp echo change is already done, so in addition to your suggested patch, we simply have to substract the old ttl and add the new ttl to the checksum and that should be it.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 18 Sep 2007 09:09:26 AM UTC, comment #5: 


>Is it? From reading the code, ICMP_TTL is passed to ip_output_if, but is not used there, since it is only used if 'dest != IP_HDRINCL', and dest is IP_HDRINCL in this case! In my opinion, the header must be completely filled in icmp_input(), so the patch in buf #19580 in fact didn't fix the problem... Or am I wrong?


No, it's seems you're right. So, a patch like this is necessary?

    iphdr->src.addr = iphdr->dest.addr;
    iphdr->dest.addr = tmpaddr.addr;
+   IPH_TTL_SET(iphdr, ICMP_TTL);
    ICMPH_TYPE_SET(iecho, ICMP_ER);

>But the checksum can still be adjusted (which is much faster), instead of being completely recalculated!


Yes, but in this case, the adjustment is more complex (icmp echo change, icmp checksum, and ip ttl)?

Frédéric Bernon <fbernon>
Group Member
Tue 18 Sep 2007 08:56:56 AM UTC, comment #4: 


> If I don't do any mistake, this problem is already fixed by Jonathan...


Is it? From reading the code, ICMP_TTL is passed to ip_output_if, but is not used there, since it is only used if 'dest != IP_HDRINCL', and dest is IP_HDRINCL in this case! In my opinion, the header must be completely filled in icmp_input(), so the patch in buf #19580 in fact didn't fix the problem... Or am I wrong?

> Oops, typo. I meant "Recalculate the IP Checksum".


Ah, OK. It's right that icmp_input() has to calculate the checksum, if ip_output_if is called with dest == IP_HDRINCL, the checksum isn't re-calculated! But the checksum can still be adjusted (which is much faster), instead of being completely recalculated!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 18 Sep 2007 08:49:48 AM UTC, comment #3: 


>In my code (version 1.2) the call to ip_output_if() is:
>
>ip_output_if(p, &(iphdr->src), IP_HDRINCL,
>IPH_TTL(iphdr), 0, IP_PROTO_ICMP, inp);


Take a look to http://savannah.nongnu.org/bugs/?19580, "bug #19580 : ICMP TTL is not decremented"...

If I don't do any mistake, this problem is already fixed by Jonathan...


Frédéric Bernon <fbernon>
Group Member
Tue 18 Sep 2007 08:45:04 AM UTC, comment #2: 


> But I'm not really sure what you mean with:
>
>> The LwIP code should set the TTL and then recalculate
>> (rather than just "adjust") the TTL.


Oops, typo. I meant "Recalculate the IP Checksum".

The ICMP code has the following, so I assumed it was in
charge of the checksum. It is likely that ip_output()
actually recalculates, in which case the following could
be removed:

    /* adjust the checksum */
    if (iecho->chksum >= htons(0xffff - (ICMP_ECHO << 8))) {
      iecho->chksum += htons(ICMP_ECHO << 8) + 1;
    } else {
      iecho->chksum += htons(ICMP_ECHO << 8);
    }

In my code (version 1.2) the call to ip_output_if() is:

    ip_output_if(p, &(iphdr->src), IP_HDRINCL,
     IPH_TTL(iphdr), 0, IP_PROTO_ICMP, inp);

That passes in "IPH_TTL(iphdr)" which looks to me to be
the TTL in the inbound packet and not a request to set to
the default (pcb->ttl) or probably (ICMP_TTL).

Tom Evans <tom_evans>
Tue 18 Sep 2007 06:11:48 AM UTC, comment #1: 

This bug doesn't seem to exist because 'RFC792 has been slavishly followed', but because of incorrect use of ip_output_if(). ICMP_TTL is passed to this function, but because IP_HDRINCL is also passed, the TTL is not changed in the existing IP header.

I think what the programmer meant was setting the TTL for every outgoing ICMP packet to ICMP_TTL. But I'm not really sure what you mean with:

> The LwIP code should set the TTL and then recalculate
> (rather than just "adjust") the TTL.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 18 Sep 2007 04:46:51 AM UTC, original submission:  

LwIP doesn't reset the TTL in ICMP Echo Replies.

This means that when using Windows Tracert, the route is
measured as "twice as long as it should be". I'm currently
over 20 hops away from an LwIP implementation. It is less
than 0.5m away from me physically, but packets are taking
a double trip over the Internet to another country and
through an RF link to get to it. The edited (...)
Traceroute (IP addresses obscured with "xxx") is:

C:>tracert -d -h 60 61.xxx.xxx.xxx

Tracing route to 61.xxx.xxx.xxxover a maximum of 60 hops

  1     1 ms     1 ms     1 ms  10.xxx
  2     1 ms     2 ms     1 ms  210.xxx
  3    14 ms    13 ms    13 ms  210.xxx
...
 10    28 ms    29 ms    31 ms  202.xxx
...
 19   271 ms   271 ms   271 ms  210.xxx
 20                     *     Request timed out.
 21                     *     Request timed out.
...
 48                     *     Request timed out.
 49  1253 ms  1876 ms  2370 ms  61.xxx.xxx.xxx

RFC792 has been slavishly followed. It says "swap the
addresses and send it back". This omission wasn't
picked up on in RFC1122 and 1123 (like I thought it was).
There's discussion in comp.protocols.tcp-ip in 1987 and
1999 on this:

http://groups.google.com/group/comp.protocols.tcp-ip/browse_thread/thread/662fabcdcfb753a6/

http://groups.google.com/group/comp.protocols.tcp-ip/msg/ac1deb7ea4c6b1ea?dmode=source&hl=en

The "modern concensus" is that ICMP should reset the TTL
in the ICMP Echo Reply to whatever is configured on that
host.

The LwIP code should set the TTL and then recalculate
(rather than just "adjust") the TTL.

Tom Evans <tom_evans>

 

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

Attached Files
file #13990:  icmp.pcap added by fbernon (744B - 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 fbernon (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by tom_evans (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-09-19 fbernon Attached File- Added icmp.pcap, #13990
    2007-09-19 goldsimon CategoryNone IPv4
        Item GroupNone Faulty Behaviour
        StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code