Use of little-endian `BitFields` can lead to incorrect packet dissection

jpollard-jumptrading commented 3 years ago

Brief description

Little-endian BitFields seem to lead to incorrect packet dissection when combined with other Field types. I've a minimum failing example, and I would appreciate it if someone with greater knowledge of Scapy could take a look and confirm whether this is a real bug, or if I'm doing something wrong.

Environment

Scapy version: b5b6e64
Python version: 2.7.7
Operating System: RHEL 7.5

How to reproduce

import scapy.all as scapy
class BadPacket(scapy.Packet):
    fields_desc = [
        scapy.BitField("c", 0, 5, tot_size=-16),
        scapy.BitField("b", 0, 2),
        scapy.BitField("a", 0, 9, end_tot_size=-16),
        scapy.LEShortField("d", 0),
    ]

bad = BadPacket()
bad.a = 1
bad.b = 2
bad.c = 3
bad.d = 0xDEAD

Actual result

In [9]: bad.show()
###[ BadPacket ]###
  c         = 3
  b         = 2
  a         = 1
  d         = 57005

In [10]: bad.show2()
###[ BadPacket ]###
  c         = 27
  b         = 3
  a         = 173
  d         = 284

Expected result

This is a very simple Packet with no computed fields: I would expect bad.show() to print the same as bad.show2().

In [9]: bad.show()
###[ BadPacket ]###
  c         = 3
  b         = 2
  a         = 1
  d         = 57005

In [10]: bad.show2()
###[ BadPacket ]###
  c         = 3
  b         = 2
  a         = 1
  d         = 57005

Related resources

Consider a little endian packet which looks like this:

Byte Offset:               0x1│            0x0
        Bit:   7 6 5 4 3 2 1 0│7 6 5 4 3 2 1 0
              ┌─────────┬───┬─────────────────┐
              │    c    │ b │        a        │ 0x00
              └─────────┴───┴─────────────────┘

Note that this is almost identical to the "Example - low endian" in the docstring for BitField, and can be modelled as follows:

class GoodPacket(scapy.Packet):
    fields_desc = [
        scapy.BitField("c", 0, 5, tot_size=-16),
        scapy.BitField("b", 0, 2),
        scapy.BitField("a", 0, 9, end_tot_size=-16),
    ]

Note that we observe the following behaviour:

In [1]: good = GoodPacket()

In [2]: good.a = 1

In [3]: good.b = 2

In [4]: good.c = 3

In [5]: str(good)
Out[5]: '\x01\x1c'

In [6]: good.show()
###[ Goodacket ]###
  c         = 3
  b         = 2
  a         = 1

In [7]: good.show2()
###[ GoodPacket ]###
  c         = 3
  b         = 2
  a         = 1

Now I'd like to add an additional field, so that we get the following:

Byte Offset:               0x1│            0x0
        Bit:   7 6 5 4 3 2 1 0│7 6 5 4 3 2 1 0
              ┌─────────┬───┬─────────────────┐
              │    c    │ b │        a        │ 0x00
              ├─────────┴───┴─────────────────┤
              │               d               │ 0x02
              └───────────────────────────────┘

I'm modelling this as:

class BadPacket(scapy.Packet):
    fields_desc = [
        scapy.BitField("c", 0, 5, tot_size=-16),
        scapy.BitField("b", 0, 2),
        scapy.BitField("a", 0, 9, end_tot_size=-16),
        scapy.LEShortField("d", 0),
    ]

Unfortunately, I now observe the following:

In [1]: bad = BadPacket()

In [2]: bad.a = 1

In [3]: bad.b = 2

In [4]: bad.c = 3

In [5]: bad.d = 0xDEAD

In [6]: str(bad)
Out[6]: '\x01\x1c\xad\xde'

In [7]: bad.show()
###[ BadPacket ]###
  c         = 3
  b         = 2
  a         = 1
  d         = 57005

In [8]: bad.show2()
###[ BadPacket ]###
  c         = 27
  b         = 3
  a         = 173
  d         = 284

Everything is fine up until the final show2(), which builds the raw packet, and tries to dissect it back into a BadPacket. I haven't had chance to look into the implementation properly, but I suspect there is a bug lurking somewhere around there.

jpollard-jumptrading commented 3 years ago

Closing in favour of https://github.com/secdev/scapy/issues/3176: this is a documentation error.

kamichal commented 2 years ago

Oh, this is not a documentation error. Bit fields are actually not documented, BTW.

The implementation for little endian decoding is incorrect when the first bitfield has uneven amount of bits, larger than 8. I already spent a whole day, debugging it. Calculations of bit shifts and masks are incorrect.

I could fix it and cover with tests if you are open for accepting my PR (please declare). Otherwise, I can just reimplement addfield and getfield methods on my own for LE use only.

secdev / scapy