pcapgo/read.go: Shouldn't ReadPacketData try to peek the packet header to check for corruption before reading?

[assafmo]

In this code section:

gopacket/pcapgo/read.go

Lines 119 to 140 in a35e09f

	func (r *Reader) ReadPacketData() (data []byte, ci gopacket.CaptureInfo, err error) {
	if ci, err = r.readPacketHeader(); err != nil {
	return
	}
	if ci.CaptureLength > int(r.snaplen) {
	err = fmt.Errorf("capture length exceeds snap length: %d > %d", 16+ci.CaptureLength, r.snaplen)
	return
	}
	data = make([]byte, ci.CaptureLength)
	_, err = io.ReadFull(r.r, data)
	return data, ci, err
	}
	func (r *Reader) readPacketHeader() (ci gopacket.CaptureInfo, err error) {
	if _, err = io.ReadFull(r.r, r.buf[:]); err != nil {
	return
	}
	ci.Timestamp = time.Unix(int64(r.byteOrder.Uint32(r.buf[0:4])), int64(r.byteOrder.Uint32(r.buf[4:8])*r.nanoSecsFactor)).UTC()
	ci.CaptureLength = int(r.byteOrder.Uint32(r.buf[8:12]))
	ci.Length = int(r.byteOrder.Uint32(r.buf[12:16]))
	return
	}

I think maybe ReadPacketData should peek 16 bytes, check for corruption according to the packet header specs, and if there is corruption then skip 1 bytes.

My thinking is that maybe the next good packet header can start somewhere in the last 15 bytes of the 16 bytes read.

Right now, if there is corruption somewhere in the input file, pcapgo/read.go reads in increments of 16 bytes and might miss on good packets.

This of course will be much slower when dealing with a corrupted pcap file, but OTOH might find more good packets otherwise missed.

[assafmo]

pcapfix also skips 16 bytes just like pcapgo/read.go.

I'd love to get some explanation/logic behind this behavior.

[notti]

I think maybe ReadPacketData should peek 16 bytes, check for corruption according to the packet header specs, and if there is corruption then skip 1 bytes.

My thinking is that maybe the next good packet header can start somewhere in the last 15 bytes of the 16 bytes read.

The big problem here is that it is not possibly or really complicated to actually verify a pcap packet header (the file header can somewhat be verified a bit, since the first 4 bytes need to match one of magic numbers).
First, there is no specification. As far as I know the pcap-format originated from the source code of libpcap and it is the reference implementation (the wireshark link you provided is just a documentation of pcap-files discovered in the wild and existing source code). This lead to several different variants (as also mentioned in the wireshark wiki link). This can lead to e.g. the timestamp field being nanosecond although microsecond is expected, or wrong snaplength (see

gopacket/pcapgo/read.go

Line 162 in a35e09f

// This snaplen situation can happen when a pcap writer doesn't truncate packets to the snaplen size while writing packets to file.

).
Second, there is no field which can actually be verified. Timestamp can be anything and also contain huge jumps due to timing issues on the capture device or concatenated pcap files. Length can experience the above mentioned issues.

What pcapfix does is just apply some heuristics of things they think should be sane (like timestamp not having jumps bigger than a day).

A better way would be to verify the packet data (e.g. protocols like ethernet (if fcs was captured), ip4, tcp, udp contain a checksum).

Right now, if there is corruption somewhere in the input file, pcapgo/read.go reads in increments of 16 bytes and might miss on good packets.

Actually it reads it in increments of 16 bytes + what the length field contains.

pcapfix also skips 16 bytes just like pcapgo/read.go.

I'd love to get some explanation/logic behind this behavior.

pcapgo/read.go doesn't do any skipping. In readPacketHeader 16 bytes are read into r.buf (r.buf has a length of 16), because a pcap header is 16 bytes (8 bytes timestamp + 4 bytes CaptureLength + 4 bytes Length). This is followed by decoding said fields. Since the header is directly followed by the packet data, ReadPacketData copies ci.CaptureLength bytes to data. For the next packet the whole process needs to be done again.

As for pcapfix I can only guess that they are trying to skip the possibly corrupted header they detected.

---

I think the whole thing would slow down the most frequent case (having a non-corrupted pcap file) and overcomplicate the reading code. Additionally, pre-knowledge of the file, or handling the actual packet data might be needed. Therefore, I think best course of action would be to go the same way as pcapfix and create something that can write repaired versions of corrupted files, or use pcapfix.

[assafmo]

The big problem here is that it is not possibly or really complicated to actually verify a pcap packet header (the file header can somewhat be verified a bit, since the first 4 bytes need to match one of magic numbers).

I guess you are right. We don't have enough data in the packet header to detect corruption. I wish there was magic number in each packet header. 😞

First, there is no specification. As far as I know the pcap-format originated from the source code of libpcap and it is the reference implementation (the wireshark link you provided is just a documentation of pcap-files discovered in the wild and existing source code). This lead to several different variants (as also mentioned in the wireshark wiki link).

I did not know this. Thank you!

This can lead to e.g. the timestamp field being nanosecond although microsecond is expected, or wrong snaplength (see

gopacket/pcapgo/read.go

Line 162 in a35e09f

// This snaplen situation can happen when a pcap writer doesn't truncate packets to the snaplen size while writing packets to file.

)

LOL 🤣 I wrote this comment.

Right now, if there is corruption somewhere in the input file, pcapgo/read.go reads in increments of 16 bytes and might miss on good packets.

Actually it reads it in increments of 16 bytes + what the length field contains.

Actually it reads also what the length field contains only if it thinks the packet is ok:

gopacket/pcapgo/read.go

Lines 118 to 130 in a35e09f

	// ReadPacketData reads next packet from file.
	func (r *Reader) ReadPacketData() (data []byte, ci gopacket.CaptureInfo, err error) {
	if ci, err = r.readPacketHeader(); err != nil {
	return
	}
	if ci.CaptureLength > int(r.snaplen) {
	err = fmt.Errorf("capture length exceeds snap length: %d > %d", 16+ci.CaptureLength, r.snaplen)
	return
	}
	data = make([]byte, ci.CaptureLength)
	_, err = io.ReadFull(r.r, data)
	return data, ci, err
	}

pcapfix also skips 16 bytes just like pcapgo/read.go.
I'd love to get some explanation/logic behind this behavior.

pcapgo/read.go doesn't do any skipping. In readPacketHeader 16 bytes are read into r.buf (r.buf has a length of 16), because a pcap header is 16 bytes (8 bytes timestamp + 4 bytes CaptureLength + 4 bytes Length). This is followed by decoding said fields. Since the header is directly followed by the packet data, ReadPacketData copies ci.CaptureLength bytes to data. For the next packet the whole process needs to be done again.

You are right. By "skipping" I meant "saying the packet is corrupted and moves on to read the next packet".

[assafmo]

I think the whole thing would slow down the most frequent case (having a non-corrupted pcap file) and overcomplicate the reading code.

Yes, this will complicate the code, but it will slow it down only in a corrupted pcap file.

[assafmo]

pcapfix also skips 16 bytes just like pcapgo/read.go.

I'd love to get some explanation/logic behind this behavior.

Actually I was wrong. pcapfix checks byte by byte for the next valid packet header.

Rup0rt/pcapfix#14 (comment)

[notti]

Ah I had a look at the source code when I wrote my first answer, but couldn't that figure out from a fast glimpse.

I think the whole thing would slow down the most frequent case (having a non-corrupted pcap file) and overcomplicate the reading code.

Yes, this will complicate the code, but it will slow it down only in a corrupted pcap file.

Since you need the checks to find out if it is corrupted, it would slow also the non-corrupted case down. Checking length vs capture length (like you did in #551) definitely makes sense, and as far as I can remember, libpcap does the same.

Another possibility to do what you want (read broken pcaps), would be to provide some methods, with which a program using the API could achieve this goal. E.g. add a Seek method and then provide a program in the examples directory. Said program could then basically use ReadPacketData, and if it fails or if additional checks fail use Seek to go back + offset and then continue with the process.

[assafmo]

A Seek method is a nice idea. 👍