PacketWriter fails to correctly serialize crafted packets

[Vociferix]

Unless I am missing something about the intended functionality, PacketWriter appears to have a bug where modifed packets and packets crafted from scratch fail to be serialized correctly. It's simplest to show an example, I think.

The following results in a malformed packet in wireshark. The serialized packet appears to simply be cut short in the middle of the source IPv4 address.

PacketWriter writer("test.pcap", DataLinkType<EthernetII>());

auto frame = EthernetII("ff:ff:ff:ff:ff:ff", "de:ad:be:ef:12:34")
             / IP("127.0.0.1", "127.0.0.1")
             / UDP(1234, 4321)
             / RawPDU("test");

writer.write(frame);

Wireshark shows:

0000   ff ff ff ff ff ff de ad be ef 12 34 08 00 45 00   ...........4..E.
0010   00 20 00 01 00 00 80 11 3c ca 7f 00               . ......<...

However, I can work around this problem with the following, which causes the packet to be written out correctly:

PacketWriter writer("test.pcap", DataLinkType<EthernetII>());

auto frame = EthernetII("ff:ff:ff:ff:ff:ff", "de:ad:be:ef:12:34")
             / IP("127.0.0.1", "127.0.0.1")
             / UDP(1234, 4321)
             / RawPDU("test");

auto _ = frame.serialize(); // prepare_for_serialize() the culprit?
writer.write(frame);

Wireshark this time shows the full packet as expected:

0000   ff ff ff ff ff ff de ad be ef 12 34 08 00 45 00   ...........4..E.
0010   00 20 00 01 00 00 80 11 3c ca 7f 00 00 01 7f 00   . ......<.......
0020   00 01 10 e1 04 d2 00 0c 04 47 74 65 73 74 00 00   .........Gtest..
0030   00 00 00 00 00 00 00 00 00 00 00 00               ............

Other information:

• libtins was built from source off of the v4.2 tag
• Only tried this on Linux
• Similar problems with the same workaround occur when modifying packets read from a Sniffer or FileSniffer and writing the modified packet to a PCAP, but only when the packet's length is changed

My use case involves a lot of testing with PCAPs, having pre-recorded PCAPs as test input, and writing results to PCAPs for further analysis and verification, before eventually testing on a live network, so this problem is a pretty big nuisance. I can work around it as shown above, but it appears to be a bug. Do let me know if I'm using it incorrectly in some way though.

[Vociferix]

Turns out, the solution to this problem is quite simple. It does appear to be a bug:

The problem is in the PacketWriter::write function, which is shown below as it exists in the source currently, except with a couple comments I've added.

void PacketWriter::write(PDU& pdu, const struct timeval& tv) {
    struct pcap_pkthdr header;
    memset(&header, 0, sizeof(header));
    header.ts = tv;
    header.len = static_cast<bpf_u_int32>(pdu.advertised_size()); // <- here is the problem
    PDU::serialization_type buffer = pdu.serialize(); // <- this updates the value returned by advertised_size()
    header.caplen = static_cast<bpf_u_int32>(buffer.size());
    pcap_dump((u_char*)dumper_, &header, &buffer[0]);
}

The problem is solved by moving the call to serialize() to before the call to advertised_size(). I intend to create a pull request with this change.

[mfontanini]

I see the issue and this is a problem because that line was specifically put there because of this issue. The line has to be there to fix that problem but causes this one. The two conflicting use cases are:

• Create a packet from scratch and try to write it (your case). The length for protocols that have a dynamic size (e.g. IP) is calculated on PDU::serialize so you obviously need to have that value before you use it and that's calculated during serialization.
• On the other hand, there's the problem of getting a packet on the network which has a capture length < the actual length of the packet. In this case, the concrete length (capture length) is only known after serializing but the "advertised length" is known before hand.

I don't think there's a pretty way of solving this but I think something like this could work:

• Keep this as it is, storing len before and then serializing.
• After serialization, check if the size of the buffer is different from the original advertised_size. If the caplen size is now greater than the len, then updated the len to be caplen.
• This way you guarantee that the length of the packet is >= the capture length. It should be impossible to have a packet of size X but you captured anything > X.

In terms of code, I think I'd just do this right before calling pcap_dump:

// The length of the packet can't be lower than capture length
header.len = max(header.len, header.caplen);

Does this make sense? I think it should work for both cases but please double check.

[Vociferix]

I actually had a feeling there would be some issue like that. Your solution would fix my problem and maintain the functionality in the issue you referenced, but it seems like there may be a need for a more general solution. For example, there could be something like a Packet::caplen() function (not sure if that name is appropriate, but you get the idea), and the overload of PacketWriter::write that takes a Packet would use that information. I haven't dug enough into the libtins code to know how feasible that is, though. You could do something similar for PDU, but it seems to me that it makes more sense to be on Packet, especially since Packet already stores the timestamp portion of the packet header.

That said, I think your solution should go in either way, since it header.len < header.caplen doesn't make logical sense, as far as I can figure. I'll try it out on my code base and report back if I have any problems.

[Vociferix]

Ok, so I added the line you suggested, and it does fix my problem. However, I realized that coincidentally it just happens that my code increases the size of all the packets it modifies. I decided to try an example where the size of the packet is decreased (my project will eventually need to make changes that result in a net decrease in size).

PacketWriter writer("test.pcap", DataLinkType<EthernetII>());

auto frame = EthernetII("ff:ff:ff:ff:ff:ff", "de:ad:be:ef:12:34")
             / IP("127.0.0.1", "127.0.0.1")
             / UDP(1234, 4321)
             / RawPDU("this is a long udp payload");

writer.write(frame);

UDP& udp = frame.rfind_pdu<UDP>();
udp.inner_pdu(RawPDU("short"));

writer.write(frame);

While this does write the data of the second packet correctly, the packet header reports that header.len > header.caplen. Wireshark says "Packet size limited during capture" for that second packet. Obviously, this is a minor complaint, but I'm not sure what the effect this will have if I then read that pcap with FileSniffer.

I still think a solution along the lines of what I described in my previous comment is called for. I'd be happy to implement something when I have time, depending on what you think. I'm in no particular rush, so if you want to stew on it, feel free.