search

kbandla / dpkt

fast, simple packet creation / parsing, with definitions for the basic TCP/IP protocols
Other
1.08k stars 270 forks source link

Fixed IEEE 802.11 Beacon byte ordering. #628

Closed rpcope1 closed 2 years ago

rpcope1 commented 2 years ago

When using dpkt, I noticed that the Beacon intervals, timestamps, and beacon capability field were being decoded in the wrong byte order. This change should address this, and adds a test to verify some basic level of correctness.

rpcope1 commented 2 years ago

After thinking about other possible byte ordering issues for 802.11, it looks like the fraq_seq field is also decoded in the wrong byte order, though I have no idea what the implications of altering (fixing?) that may be for downstream users. I've added some extra code after looking at #622 to allow easy extraction of the correct fragment and sequence numbers, where applicable.

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.0006%) to 99.816% when pulling 37419b897df1b5d8c8f0bbb0190deb2c0048942d on cope-systems:ieee802.11-fixes into a39d5189613de3f57485b597cc73f7d328e6d6b0 on kbandla:master.

rpcope1 commented 2 years ago

I guess the question I have at this point, is that my reading of the 802.11 specification seems to indicate that most of the fields are network byte order. It looks like a lot of the parsing code is unpacking the data as little endian. I think that might be worth addressing from a correctness standpoint, but that also may break existing users that expect the current behavior. Is there a process or procedure for handling this?

obormot commented 2 years ago

I guess the question I have at this point, is that my reading of the 802.11 specification seems to indicate that most of the fields are network byte order. It looks like a lot of the parsing code is unpacking the data as little endian. I think that might be worth addressing from a correctness standpoint, but that also may break existing users that expect the current behavior. Is there a process or procedure for handling this?

I'm going to merge this PR as the work here looks complete. Suggesting we move this question into a new issue @rpcope1 . Thanks!