RadioTap - EXT bit ignored, begins parsing data too early

[llamasoft]

It seems that the RadioTap parser is ignoring the EXT bit (bit 31) in the RadioTap "present" word. This results in it parsing the subsequent "present" word(s) as payload data, leading to incorrect results.

Here's the hex dump of a RadioTap header:

>>> binascii.hexlify( p.original[:p.len] )
'00002000ae4000a0200800a02008000000029e09a000a2002b00000000000001'

Here's how it was parsed:

###[ RadioTap dummy ]###
  version   = 0
  pad       = 0
  len       = 32
  present   = Flags+Rate+Channel+dBm_AntSignal+Lock_Quality+RXFlags+RadiotapNS+Ext
  Flags     = pad
  Rate      = 8
  Channel   = 40960
  ChannelFlags= CCK+GFSK
  dBm_AntSignal= -256dBm
  notdecoded= '\x00\x00\x02\x9e\t\xa0\x00\xa2\x00+\x00\x00\x00\x00\x00\x00\x01'

Here's how it should have been parsed:

00          # Version
00          # Padding
2000        # Length
ae4000a0    # Present word, EXT bit set, another word incoming
200800a0    # Present word, EXT bit set, another word incoming
20080000    # Present word, no EXT bit set, data begins after this
00          # Flags
02          # Rate
9e09        # Channel (2.462 GHz)
a000        # Channel Flags (CCK + 2GHz)
...

The extended present words don't even need to be parsed per-se, they just need to be consumed so the data parsing begins at the correct position.

[gpotter2]

True, scapy’s implementation does not currently supports it. It however should (I had not seen that part when we reimplemented the tags dissection)

On the TODO list.

If you want, you can have a look at how it’s done in scapy/layers/dot11.py and submit a PR :)

[llamasoft]

Oh trust me, I tried, but nothing I came up with was terribly elegant. Maybe it's because I don't know scapy quite as well, but I couldn't find a field definition that could be repurposed for parsing the extended present words. FieldListField looked promising, but the issue is that the extended words are recursive. The count or length depends on parsing the previous word, but the length and count parameters want a value at time of initialization.

[gpotter2]

Ok I will have a look 😄

[gpotter2]

Could you please share a pcap of some packets containing this EXT tag please ? Thanks !

[llamasoft]

Here you go: rt_ext.pcap
The file extension has a .txt added so GitHub would accept it. In case GitHub mangles it, the sha1 hash is fa8f9392a0534d6ae06e067bcbe5beb48ab55652.

These were captured with an RTL8812AU using Kali Linux.

[gpotter2]

Ok this should be fixed in https://github.com/secdev/scapy/pull/1466 Would be great if you could test it !

(PS: leave the issue open as a record, it will be closed when the PR is merged)

[llamasoft]

I like it! I was in the process of hacking together a pull request using a similar idea, but I like your execution better. I was using a FieldListField, but the PacketListField seems like a more appropriate type.
Plus, you have unit tests. 👍

I wish there was a way to properly number and incorporate the extended bit fields in the main packet, but I couldn't think of a way to do it. A solution where something like packet.b82 would be valid. The more I thought about it, the messier the implementation sounded though, especially if you eventually added full support for the VendorNS flag.

Either way, cheers for putting together a fix for this so quickly!

[gpotter2]

Maybe we could trick a custom field to be able to do what you want... will work on a Proof if concept

[gpotter2]

I've extended the implementation so that it detects properly b82, however, I left the lists, for two reasons:

• It was way easier and cleaner to implement
• It matches more closely the doc / wireshark, having different packs of flags

If it really is blocking for you we can look for some weirdo way, but it is very easy to use it as so:

For instance 82, one would need to call: packet.Ext[2].present.b82 (because it's in the subpack n°3 (82/31=2.5<=3))

[llamasoft]

I approve. 👍 I figured there wouldn't be a clean a way to incorporate the values into the main packet object. There'd have to be some weird catch-all logic in case someone tried to access something like b1234 with no Ext bits set.
While I don't personally have a use for the extra parsed flags, having them accessible is never a bad thing. They're certainly more useful like this than a list of notdecoded blocks.