IETF-OPSAWG-WG / draft-ietf-opsawg-pcap

PCAP next generation file format specification
Other
272 stars 62 forks source link

Many pcapng implementations got the bit order for epb_flags backwards #57

Closed guyharris closed 5 years ago

guyharris commented 5 years ago

Section 4.3.1 "Enhanced Packet Block Flags Word" of the pcapng spec says:

The word is encoded as an unsigned 32-bit integer, using the endianness of the Section Header Block scope it is in. In the following table, the bits are numbered with 0 being the most-significant bit and 31 being the least-significant bit of the 32-bit unsigned integer.

with bits 0 and 1 being the packet direction.

That appears to indicate that the direction field is at the top of the flags word, so that, for example an inbound packet, with the FCS length field zero, the reception type unknown, and no link-layer-dependent error, would have a flags word of 0100 0000 0000 0000 0000 0000 0000 0000, or 0x40000000.

However:

so there are, depending on whether you consider the two pieces of Wireshark code as one implementation or two (they were done by different people, but maybe the person who made the text2pcap changes was influenced by my screwup in the pcapng reading code), two or three implementations of pcapng that treat bit 0 as the low-order bit.

There may be, or have been, another implementation that did so; the capture attached to Wireshark bug 11665 has ISDN LAPD packets in it, and the direction field at the bottom of the flags field has a value of 1 for network->user packets and 2 for user->network packets, which would make sense if the machine on which the capture was being done was "terminal equipment" (or whatever the right ISDN term is), i.e. if it's the "user".

I don't know whether there are any implementations that treat bit 0 as the high-order bit. None of the pcapng files I have on hand appear to do so - they all seem to have that information in the low-order bits.

So, whilst I really hate to do this, would the least bad solution be to change the spec to say that bit 0 is the low-order bit?

If there truly are no implementations that treated it as the high-order bit, that would work, with the risk that somebody develops a 0-is-the-high-order-bit implementation based on the current or earlier state of the specification.

If there are only private, internal implementations that do so, that would cause pain to some unknown number of implementors, but they wouldn't have to worry about the general user base of programs that handle (or that will handle in the future) the flags field.

If there are public implementations that do so, we may be stuck with heuristics, as there are also public implementations that treat bit 0 as the low-order bit; as long as nobody used the "link-layer-dependent flags" part of the flags field, we could treat any flags word with the upper 16 bits non-zero as coming from a "bit 0 as the high-order bit" implementation and any flags word with the lower 16 bits non-zero as coming from a "bit 0 as the low-order bit" implementation. If both are zero, it obviously doesn't matter; if both are non-zero, that'll need more heuristics, and they'd be more likely to fail.

AndersBroman commented 5 years ago

I'd vote for changing the spec at this stage.

guyharris commented 5 years ago

Note, for what it's worth, that:

So this somewhat suggests that it's not inconceivable that still others may have made the same error.

guyharris commented 5 years ago

A quick look at the pktdump source (I'm not sure how to get the full source, but I managed to see a code diff that showed code looking at the EPB flags field) indicates that it also treats bit 0 as the low-order bit.

grimbeaver commented 5 years ago

For what it's worth I can tell you that when I implemented the (limited) support of pcapng in Viavi Solutions Observer I know I did not implement the epb_flags and I doubt anyone has gone back and added it since I left.

packetfoo commented 5 years ago

I just checked the TraceWrangler pcapng loading code, and I do a "AND 0x03" operation to get the direction flags, so if I'm not mistaken I do the same as the others, meaning 0x00000001 for an inbound packet.

I say let's do the spec change, as the risk of breaking existing other implementation seems low, especially since packet those flags are not mission critical under most circumstances I guess.

Good job spotting this, @guyharris !

guyharris commented 5 years ago

OK, changed the spec in 3c35b6abf9171e767e4b2470e691c22346b7105e to match reality.