secdev / scapy

Scapy: the Python-based interactive packet manipulation program & library.
https://scapy.net
GNU General Public License v2.0
10.29k stars 1.99k forks source link

Modification of Dot11Elem consistency fixups #4415

Closed rkinder2023 closed 3 weeks ago

rkinder2023 commented 3 weeks ago

Brief description

The Dot11Elem class is used to encapsulate information elements as per the 802.11 standard. These consist of a one byte type, one byte length, then variable data. If one modifies the 'info' field (data) of an IE, the length field may need to be recalculated if the size of the info field changes. If this is not done, the resulting packet created will be invalid due to IE length mismatch.

The fix for this issue is to modify the Dot11Elem.setattr call to update the 'len' field if the size of the info field changes.

The following diff achieves this.

diff --git a/scapy/layers/dot11.py b/scapy/layers/dot11.py
index 8ed4d38c..08f6d54c 100644
--- a/scapy/layers/dot11.py
+++ b/scapy/layers/dot11.py
@@ -1021,6 +1021,9 @@ class Dot11Elt(Packet):
             # Will be caught by __slots__: we need an extra call
             try:
                 self.setfieldval(attr, val)
+                # Need to modify length if the length of the info change
+                if len(val) != len(self.info):
+                    self.setfieldval("len", len(val))
             except AttributeError:
                 pass
         super(Dot11Elt, self).__setattr__(attr, val)

Scapy version

b4bf3d62d5aa6f160a0768ca3ad598fb415f0994

Python version

3.12.2

Operating system

MacOS Sonoma 14.4

Additional environment information

No response

How to reproduce

Simple script to show the problem. The solution is presented earlier in the report.


from scapy.all import *

# Single beacon packet, RadioTap header
pkt=RadioTap(b"\x00\x00$\x00o\x08\x00@\xef]-\xdd\x00\x00\x00\x00\x12\x0c<\x14@\x01\xbb\xa5\x01\x05\x00\x10\x18\x03\x04\x00\x00\x00\x00\x00\x80\x00\x00\x00\xff\xff\xff\xff\xff\xff\\\xe91\x00\xa7\xa2\\\xe91\x00\xa7\xa2p86P\x1d\xce2\x05\x00\x00d\x00\x11\x15\x00\x0fTP-Link_A7A4_5G\x01\x08\x8c\x12\x98$\xb0H`l\x03\x010\x05\x04\x00\x01\x00\x00\x07\nDE $\x08\x17d\x0b\x1e\x00 \x01\x03#\x02\x16\x000\x14\x01\x00\x00\x0f\xac\x04\x01\x00\x00\x0f\xac\x04\x01\x00\x00\x0f\xac\x02\x0c\x00F\x05s\xd0\x00\x00\x0c-\x1a\xef\t\x03\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00=\x160\x07\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x7f\x08\x04\x00\x0f\x02\x00\x00\x00@\xbf\x0c\xb29\x813\xfa\xff\x00\x00\xfa\xff\x00 \xc0\x05\x01*\x00\xfc\xff\xc3\x05\x03....\xff\x1d#\t\x01\x08\x9a@\x10\x04`H\x88\x1dA\x81\x0c\x11\x08\x00\xfa\xff\xfa\xffy\x1c\xc7q\x1c\xc7q\xff\x07$\xf4?\x00\x11\xfc\xff\xff\x02'\x03\xff\x0e&\x00\x03\xa4\xff'\xa4\xffBC\xffb2\xff\xdd\x17\x8c\xfd\xf0\x01\x01\x02\x01\x00\x02\x01\x01\x03\x03\x01\x01\x00\x04\x01\x01\t\x02\x00\x00\xdd\x18\x00P\xf2\x02\x01\x01\x80\x00\x03\xa4\x00\x00'\xa4\x00\x00BC^\x00b2/\x00\xdd\x16\x8c\xfd\xf0\x04\x00\x00ILQ\x03\x02\tr\x01\x00\x00\x00\x00\xfb\xff\x00\x00\xdd\x07\x8c\xfd\xf0\x04\x01\x01\x01\xdd\x1e\x00\x1d\x0f\x10\x01c\x00\x00\\\xe91\x00\xa7\xa4\\\xe91\x00\xa7\xa4;\xd2\x00\x00\xa7\xa4\x00\x01\x00\x00\xdd\x1d\x00P\xf2\x04\x10J\x00\x01\x10\x10D\x00\x01\x02\x10<\x00\x01\x03\x10I\x00\x06\x007*\x00\x01 p\xc2a\xcc")
print("SSID value:", pkt.getlayer(Dot11Elt)[0].info)
print("SSID length:", pkt.getlayer(Dot11Elt)[0].len)
pkt.getlayer(Dot11Elt)[0].info="NewSSID"
print("After update SSID")
print("SSID value:", pkt.getlayer(Dot11Elt)[0].info)
# Len should be 7, but remains as 15
print("SSID length:", pkt.getlayer(Dot11Elt)[0].len)```

### Actual result

SSID value: b'TP-Link_A7A4_5G'
SSID length: 15
After update SSID
SSID value: NewSSID
SSID length: 15

### Expected result

SSID value: b'TP-Link_A7A4_5G'
SSID length: 15
After update SSID
SSID value: NewSSID
SSID length: 7

### Related resources

_No response_
rkinder2023 commented 3 weeks ago

Hi all, I'd like to submit a PR for this, but seems I can't create branches from master. Do I need to be added into the project somehow? Please let me know.

gpotter2 commented 3 weeks ago

The behavior you're describing is by design. You can set len to None to have Scapy recompute it.

rkinder2023 commented 3 weeks ago

The behavior you're describing is by design. You can set len to None to have Scapy recompute it.

OK, makes sense, but how do I get Scapy to recompute the len field after setting to None? Is there any special command to do this? I have tried 'build()' on the full packet and the IE layer itself but it doesn't seem to update the len field.

Second question is if setting len to None, how do I create a fuzzed IE field where the info is not empty but I want to force zero length to test the robustness of IE parsing on a device?

rkinder2023 commented 1 week ago

@gpotter2, any comments here? I've looked further into this and I can confirm the build of the packet will insert the length field into the byte stream correctly, but will not set the length field within the given IE correctly (ie, still shows as None when dumping the IE). This seems counter-intuitive to me, as the 'byte' consistency is different than the printout. In the earlier case it's fine - explicit length set to make a 'bad' IE, and both the 'byte' and printout values are consistent. Does it make sense?

Example of this behaviour below:

>>> from scapy.all import *
WARNING: No IPv4 address found on anpi1 !
WARNING: No IPv4 address found on anpi0 !
WARNING: more No IPv4 address found on en3 !
>>> a=b"\x00\x07NewSSID"
>>> ssid=Dot11Elt(a)
>>> ssid
<Dot11Elt  ID=SSID len=7 info=b'NewSSID' |>
>>> ssid.info="New SSID longer"
>>> bytes(ssid)
b'\x00\x07New SSID longer' # Expected - len field unaltered
>>> ssid.len=None
>>> bytes(ssid)
b'\x00\x0fNew SSID longer'  # Expected - new length field calculated correctly.
>>> ssid
<Dot11Elt  ID=SSID len=None info=b'New SSID longer' |> # Unexpected - len field should be updated
>>>