Invalid parsing of ICMP TTL expired packet

[mfontanini]

So, by generating a packet like this using scapy:

Ether() / IP() / ICMP(type='time-exceeded') / IP() / UDP(dport=1, sport=2) / ('A' * 180)

If you then parse it using libcrafter, you get:

< Ethernet (14 bytes) :: DestinationMAC = ff:ff:ff:ff:ff:ff , SourceMAC = 00:00:00:00:00:00 , Type = 0x800 , >
< IP (20 bytes) :: Version = 4 , HeaderLength = 5 , DiffServicesCP = 0 , ExpCongestionNot = 0 , TotalLength = 236 , Identification = 0x1 , Flags = 0 , FragmentOffset = 0 , TTL = 64 , Protocol = 0x1 , CheckSum = 0x7c0e , SourceIP = 127.0.0.1 , DestinationIP = 127.0.0.1 , >
< ICMP (8 bytes) :: Type = 11 , Code = 0 , CheckSum = 0xf3cf , Length = 0 , >
< RawLayer (128 bytes) :: Payload = \x45\x0\x0\xd0\x0\x1\x0\x0\x40\x11\x7c\x1a\x7f\x0\x0\x1\x7f\x0\x0\x1\x0\x2\x0\x1\x0\xbc\xf\x80\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41>
< ICMPExtension (4 bytes) :: Version = 4 , Reserved = 321 , CheckSum = 0x4141 , >
< ICMPExtensionObject (4 bytes) :: Length = 16705 , ClassNum = 65 , CType = 65 , >
< RawLayer (72 bytes) :: Payload = AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA>

As you see, this is parsing part of the ICMP payload as ICMP extensions. I tracked this down to ICMP::ParseLayerData which does this:

    /* Non-Compliant applications don't set the Length field. According to RFC4884,
     * Compliant applications should assume no extensions in this case. However, it
     * is advised to consider a 128-octet original datagram to keep compatibility. */
    if (icmp_length == 0 && length > 128)
        icmp_length = 128;

Reading the RFC, it seems like this is a special case for non-compliant ICMP extensions that might be implemented before the RFC (referring to packets that contain ICMP extensions but not the length field).

I guess this should at least check if any valid extensions are found after that (e.g. the length field makes sense or even better, compute the checksum and make sure it matches) and otherwise, just parse the whole payload as a RawLayer.

[mfontanini]

Another thing is, this seems to be re-computing the checksum for that bogus ICMP extension and then altering the original packet buffer (which alters the ICMP payload). This ends up breaking some logic we have to match ICMP TTL expired responses, since the original packet is different from the response.

Shouldn't checksum computations be stored on the fields rather than on the underlying packet buffer?

[pellegre]

Hi Matias,

given that compliant applications are suppose to set the length field maybe is safe to remove that piece of code?

about the checksum, it should work as any other layer, values should be preserved from the original raw data until the "Craft" method is called.

in your case I see a 0x4141 value, which belongs to the original raw data. when do you see that value changing? maybe something is calling "Craft" after parsing the packet?

[mfontanini]

I think it's fairly common that the length field is not set, so I guess computing the checksum is the best way to be safe.

Regarding the original raw data being changed, you can test it doing this:

First generate the input pcap using scapy:

wrpcap('/tmp/input.pcap', Ether() / IP() / ICMP(type='time-exceeded') / IP() / UDP(dport=1, sport=2) / ('A' * 180))

Then this small snippet to parse the file:

#include <crafter.h>
#include <tins/tins.h>

using namespace Crafter;

bool callback(const Tins::PDU& pdu) {
    const auto& buffer = pdu.rfind_pdu<Tins::RawPDU>().payload();

    printf("Raw:    ");
    for (size_t i = 0; i < buffer.size(); ++i) {
        printf("%02x", buffer[i]);
    }
    printf("\n");

    Packet pkt;
    pkt.PacketFromLinkLayer(buffer.data(), buffer.size(), DLT_EN10MB);

    printf("Parsed: ");
    for (size_t i = 0; i < pkt.GetSize(); ++i) {
        printf("%02x", pkt.GetRawPtr()[i]);
    }
    printf("\n");

    return true;
}

int main(int argc, char* argv[]) {
    Tins::FileSniffer sniffer(argv[1]);
    sniffer.set_extract_raw_pdus(true);
    sniffer.sniff_loop(callback);
}

This prints:

Raw:    ffffffffffff0000000000000800450000ec0001000040017c0e7f0000017f0000010b00f3cf00000000450000d00001000040117c1a7f0000017f0000010002000100bc0f80414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141
Parsed: ffffffffffff0000000000000800450000ec0001000040017c0e7f0000017f0000010b0025e200200000450000d00001000040117c1a7f0000017f0000010002000100bc0f804141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141419145004c0000414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141

As you can see, where there should be a bunch of 0x41s, there's a 9145004c0000, that's where the checksum would be. The thing is, it looks like this modifies the internal buffer, but not the fields, since the fields still show the original value.

[pellegre]

got it now... is the internal buffer. I'll check out what's going on.

thanks :+1:

[mfontanini]

Cool, thank you!

[oliviertilmans]

As I ran into a similar bug, I eventually fixed this.

Your reported internal buffer corruption was due to pkt.GetRawPtr() (instead of pkt.GetBuffer()) which trigger a packet (re)crafting (and incidentally in your case since the packet was incorrectly parsed, it was incorrectly recrafted).

[mfontanini]

Alright, thanks! I'll change the code to use GetBuffer rather than GetRawPtr. It's kind of misleading that it crafts the packet, as I'd expect it to just return a raw pointer. There should probably be at least a comment about that before the method definition.

[oliviertilmans]

IIRC there's a note in the header file but not very clear --- I wasn't aware of that API difference either before looking into your issue... I'll explicit that in the next change/fix I push in the lib.