Scapy.contrib.modbus cannot handle packets with multiple ADU/PDU sets

[Insanitree]

Brief description

In non-spec packets where multiple ADU/PDU layers exist in parallel the first Modbus ADU Response layer consumes the entire payload. This causes registerVal fields in ModbusPDU03 and 04 responses (at least), to end up significantly larger than expected and missing other ModbusTCPResponse packets.

Scapy version

2.5.0

Python version

3.11.3

Operating system

Windows 10

Additional environment information

No response

How to reproduce

from scapy.all import *
import scapy.contrib.modbus as mb
packets = PcapReader("FailedPackets.pcap")
for pkt in packets:
    if (mb.ModbusPDU04ReadInputRegistersResponse in pkt):
                #print (str(inc) +":" + str(pkt[mb.ModbusPDU04ReadInputRegistersResponse].fields))
        pdureglen = len(pkt[mb.ModbusPDU04ReadInputRegistersResponse].fields["registerVal"])
        pduregval = (pkt[mb.ModbusPDU04ReadInputRegistersResponse].fields["byteCount"])
        #if (pdureglen != (pduregval/2)):
        print (pdureglen,int(pduregval/2),sep=":")

Actual result

132:99
Observe that we have 132 registerVal words, when we expected 99 based on bytecount

Expected result

99:99

Related resources

SamplePackets.zip

[Insanitree]

I'm working on fixing this myself, but I'm not familiar with Scapy. It appears we need two steps to a solution.
First step is properly calculating the length of each packet. in the PDU, that's as easy as assigning the length from the packet data itself to the length_from.

                   FieldListField("registerVal", [0x0000],
                                  ShortField("", 0x0000),
                                  length_from=lambda pkt: pkt.byteCount,
                                  count_from=lambda pkt: pkt.byteCount)]

In the ADU, well, I haven't figured out how to interpret the len variable from the packet during dissection without breaking the build function.

The second step is to implement extract_padding. I'll know if I get there.

[Insanitree]

Step 1: Properly calculate the length of the PDU using: length_from=lambda pkt: pkt.byteCount

Step 2: Extract PDU padding with

def extract_padding(self, s):
        pay = s[:self.byteCount]
        pad = s[self.byteCount:]
        return pay, pad

Step 3: Extract ADU Padding with

def extract_padding(self, s):
        pay = s[:self.len-1]
        pad = s[self.len-1:]
        return pay, pad

Step 4: Add a layer that contains a PacketListField

    name = "ModbusTCPResponse"
    fields_desc = [
        PacketListField("ADU_List",None,ModbusADUResponse)
        ]

Step 5: Bind the new layer like: bind_layers(TCP, ModbusTCPResponse, sport=502)
Result:

           \ADU_List  \
            |###[ ModbusADU ]### 
            |  transId   = 0x7cfe
            |  protoId   = 0x0
            |  len       = 201
            |  unitId    = 0xff
            |###[ Read Input Registers Response ]### 
            |     funcCode  = 0x4
            |     byteCount = 198
            |     registerVal= [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 4, 0, 0, 0, 0, 0, 0, 0, 475, 0, 470, 0, 19000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 25697, 26989, 110, 0, 0, 0, 0, 0, 0, 0, 12337, 48, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
            |###[ ModbusADU ]### 
            |  transId   = 0x7cff
            |  protoId   = 0x0
            |  len       = 7
            |  unitId    = 0xff
            |###[ Read Input Registers Response ]### 
            |     funcCode  = 0x4
            |     byteCount = 4
            |     registerVal= [4, 0]
            |###[ ModbusADU ]### 
            |  transId   = 0x7d00
            |  protoId   = 0x0
            |  len       = 47
            |  unitId    = 0xff
            |###[ Read Input Registers Response ]### 
            |     funcCode  = 0x4
            |     byteCount = 44
            |     registerVal= [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

This works, but I'm not convinced it's still the best way. I suspect there is a way to not use the PacketListField and still have each ADU be a sibling not a child.

Are there unit tests associated with this module?

[Insanitree]

I have run and passed the unit tests for this module. I intend to expand them in lieu of further guidance.
I have not contributed to an open source project. What is my next step here, now that I have a potential solution to present?

[gpotter2]

Sorry for the delay. You may provide a Pull Request :)