Libtins Update 1.2 breaks split IP packets

[Einar Jon Gunnarsson]

Hi
Thanks for a great library.

This might be a real edge case, but the addition to libtins 1.2 that handles split IP packets differently is breaking some things for me.
Now the type information of the ICMP packet is lost and it is only recognized as RawPDU by the system.
This breaks things so any split pings aren't recognized by the kernel.
This also applies to any split IP packets serialized by Tins (TCP/UDP/etc).

Is there a simple way of preserving the inner PDU type of split IP packets?

For now I hacked it to make the IP::write_serialization check
if (new_flag != 0xff)
before overwriting the protocol. (tins/src/ip.cpp, line 402)
but since this only seems to apply to fragmented packets,
if (!is_fragmented())
or a combination of the two should also work

Cheers
Einar Jón


Code and output below
---------------------------------------------
// Patch: "Works for me"  - no idea if other PDU types need similar treatment, or if this is the right way to do it...
diff --git a/tins/src/ip.cpp b/tins/src/ip.cpp
index 28a43b8..f3f44ff 100644
--- a/tins/src/ip.cpp
+++ b/tins/src/ip.cpp
@@ -400,7 +400,8 @@ void IP::write_serialization(uint8_t *buffer, uint32_t total_sz, const PDU* pare
                 Internals::pdu_type_to_id<IP>(inner_pdu()->pdu_type())
             );
         }
-        protocol(new_flag);
+        if (new_flag != 0xff)
+            protocol(new_flag);
         //flag(new_flag);
     }
    
-------------------------------------------------
This fake ping loopback used to work in 1.1, except for throwing an exception on ICMP packets of length 1 to 7.
(Which happens only for Pings with a payload of 1473 to 1479 +N*1500 bytes).
Now all split pings (any payload > 1473 bytes) fail

Wireshark output shows how inner PDU changes.
  0.001007    10.0.0.2 -> 10.0.0.1    IPv4 1514 Fragmented IP protocol (proto=ICMP 0x01, off=0, ID=b4d5)
  0.002807    10.0.0.2 -> 10.0.0.1    ICMP 162 Echo (ping) request  id=0x1334, seq=1/256, ttl=64
  0.025909    10.0.0.1 -> 10.0.0.2    IPv4 1514 Fragmented IP protocol (proto=Unknown 0xff, off=0, ID=b4d5)
  0.033569    10.0.0.1 -> 10.0.0.2    IPv4 162 Unknown (0xff)

Code stub below.
// return Ping
int ReturnPing(uint8_t *data, ssize_t size)
{
        int bytesSent = 0;
        Tins::EthernetII frame(data, size);
        Tins::IP *ip = frame.find_pdu<Tins::IP>();
        Tins::ICMP *ping = frame.find_pdu<Tins::ICMP>();

        if (ip != NULL && (ip->is_fragmented() // libtins 1.2
              || (ping != NULL && ping->type() == Tins::ICMP::ECHO_REQUEST)))
        {
            // turn it around
            Tins::EthernetII reply(frame.src_addr(), frame.dst_addr());
            Tins::IPv4Address tmpAddr = ip->src_addr();
            ip->src_addr(ip->dst_addr());
            ip->dst_addr(tmpAddr);
            if (ping != NULL && ping->type() == Tins::ICMP::ECHO_REQUEST) // ignore others (libtins 1.1 only)
                ping->set_echo_reply(ping->id(), ping->sequence());  // creates ping reply

            // put it all together
            reply /= *ip;

            std::cout << "ip->protocol() == " << int(ip->protocol()) << std::endl; // ip->protocol() == 1 (PROTO_ICMP) at this point, but the serialized reply still contains a RawPDU
            Tins::PDU::serialization_type serializedData = reply.serialize();
            bytesSent= Send_Back((uint8_t *) serializedData.data(), serializedData.size());
        }
        return bytesSent:
}  

[Matias Fontanini]

Hi Einar,

thank you for your bug report! The protocol id was being overwritten for no reason. I've just pushed a fix, please check if it works for you. 

I didn't apply your patch since, as you said, avoiding the 0xff flag is only okay for fragmented packets. So I added a check to do this only if the packet is fragmented.

Please note that the project has been recently moved to github, so please clone the right repository:

https://github.com/mfontanini/libtins

Please let me know if you find any other bugs.

[Einar Jón]

Hi Matias

Thanks for checking it out. I think your implementation makes the most sense.

But now that I have your attention, I'd like to ask why the EthernetII frames are now padded to 60 bytes?

ARP requests are only 42 bytes on Linux, so I noticed a growth in the serialized packets created by libtins 1.2.
I just deleted the new EthernetII::trailer_size() function of the library to "fix" it, but is there any good reason to extend EthernetII frames to at least 60 bytes?

Cheers
Einar Jón

[Matias Fontanini]

Ethernet frames must be at least 60 bytes long(plus a 4-byte FCS which
is appended by the network driver). ARP requests are not 42 bytes
long, you're seeing that because probably that padding is being
appended by the driver, after wireshark/tcpdump capture it.

Here's an interesting thread about that:

http://www.microchip.com/forums/m493473-print.aspx

You could check if this is true by sending ARP requests and capturing
them both in the receiver and transmitter hosts. For example, I'm
seeing that ARP packets sent by my host(both requests and replies) are
42 bytes long, but that's probably "an illusion" caused by what I
mentioned above.

I think what libtins is doing is right, but feel free to correct me if
I'm wrong.

> --
> You received this message because you are subscribed to the Google Groups
> "libtins" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to libtins+u...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[Einar Jón]

Interesting thread, but it says that the Ethernet hardware adds these bytes, and I'm not using any.

I'm currently using libtins with a virtual tap interface on an embedded Linux (3.2.46-omap4 armv7l) and a linux virtual machine (Linux ubuntu 3.5.0-42-generic #65~precise1-Ubuntu SMP Wed Oct 2 20:57:24 UTC 2013 i686 i686 i386 GNU/Linux)

On both setups the tap debug output prints that it only read 42 bytes when I call
$ arping <IP_ADDRESS> -I <TAP_DEVICE> # or just check the ARP request from a ping

It prints that it only read 43 bytes when I call
$ ping -s 1 <IP_ADDRESS>

AFAIK there are only 42/43 bytes in those packets, although they are handled as valid EthernetII packets.

So with a virtual interface like TUN/TAP this trailer doesn't seem to be needed - and in my use-case I'd rather be without it.
I'll just leave them out on my "branch" of the library and call it a day.

Thanks
Einar Jón

[Matias Fontanini]

Oh, I see. Then I can add some compilation switch which allows you to
disable this feature.

Even though I don't think this causes any real trouble, I understand
that you might want the result of the serialization to be exactly the
same as the input packet.

I'll add that switch later today and let you know.

[Einar Jón]

Cool

I checked what's going on with a normal eth interface, and there all the packets seem to be padded up to 60 bytes, so for normal usage 60 seems to be the minimum.
Outgoing "ping -s1 "/arp packets are smaller, but all incoming packets are >= 60b - I guess the padding is added after wireshark intercepts the packet.
So padding right away makes sense - otherwise it will be done later on in all use-cases (except mine).

As I said, I'm fine with disabling the padding on my own, but if you make a compilation switch that's cool too.

Thanks again
Einar