Packet::setRawPacket parses one layer beyond parseUntilLayer

[perdisci]

Packet::setRawPacket does not stop parsing at parseUntilLayer and instead parses the next layer after parseUntilLayer: https://github.com/seladb/PcapPlusPlus/blob/master/Packet%2B%2B/src/Packet.cpp#L77

Notice the call to curLayer->parseNextLayer(); on line 79

Fix: Replacing while ( ... && curLayer->getOsiModelLayer() <= parseUntilLayer) with while ( ... && curLayer->getOsiModelLayer() < parseUntilLayer) seems to solve the issue.

[tigercosmos]

could you provide more detail? such as the pcap file of the packet that you tried to parse.

[perdisci]

I found this bug when trying to parse packets containing malformed DNS messages: 20180203-dns.zip

pcpp::Packet packet(&rawPacket, pcpp::OsiModelTransportLayer); still causes a DNS parsing error message, indicating that parsing goes beyond the transport layer (UDP, in this case) and continues to the next layer.

The DNS parsing error is DNS layer contains more than 300 resources, probably a bad packet. Skipping parsing DNS resources, which is caused because DnsLayer::parseResources() is called, although it should not be, if parsing actually stopped at the UDP layer.

[seladb]

@perdisci I think it's a duplicate of https://github.com/seladb/PcapPlusPlus/issues/657, am I missing something?

Please see my response here on why an "extra layer" is being parsed: https://github.com/seladb/PcapPlusPlus/issues/657#issuecomment-850186314

The issue is that there might be several layers of the same OSI layer. The most trivial example I can think of is multiple VLAN layers. If we change <= to < only the first VLAN layer will be parsed. The only way to know what the next layer would be is to parse it. Maybe we can introduce "partial parsing" or "detect layer" functionality that does the minimum work needed to detect what is the next layer 🤔

[perdisci]

Ah yes, you are right, this seems like a duplicate of #657. Apologies for not noticing that.

My 2 cents: I think the correct behavior is what suggested by @weyrick in #657. For VLAN layers and similar cases at the link layer, can't you look at the header's EtherType field (or other header fields for link layers other than Ethernet) to determine the type of the next layer and decide if you do need to parse it or not, depending on the value of parseUntilLayer? https://en.wikipedia.org/wiki/IEEE_802.1Q

Also, the transport layer is well defined. If one asks to stop parsing there (and not parse the application layer payload), I think it should be feasible to honor that request. But I don't know the pcpp code base well enough to make more detailed suggestions.

For my use case (do not parse application layer) using < parseUntilLayer instead of <= is sufficient, so I fixed it in my local fork, but as you mentioned this may not generalize to all cases/layers without further changes.

Thanks for looking into this.

[seladb]

Thanks @perdisci ! yes, I think @weyrick 's idea is a nice way to fix it with minimum API changes. https://github.com/seladb/PcapPlusPlus/issues/657 is still open so anyone interested can make these changes. If that's ok with you, I think we can close this issue to avoid duplicates