From 4f30b3c25bf5af0bcd0586ab15254aa6e2310f7f Mon Sep 17 00:00:00 2001 From: ArockiaRajamanickam Date: Fri, 31 Jul 2026 03:54:22 +0530 Subject: [PATCH] modbus: stop variable arrays over-reading into the next ADU byteCount is a byte count, but FieldListField.count_from expects an element count. For the arrays whose elements are 16-bit registers that made the field consume twice as many items as the PDU actually holds, so when several ADUs arrived in one buffer the array swallowed the header of the following one and returned wrong values. Parsing two concatenated Read Input Registers responses gave registerVal [0x1111, 0x2222, 0x3333, 1, 0, 9], the trailing three shorts being the next ADU's transId, protoId and len, and the second ADU was left unrecoverable. No exception was raised. Divide byteCount by the element size for those arrays. The three arrays of byte-sized elements were already correct and are untouched. Read FIFO Queue is a special case: its byteCount also covers the FIFOCount field, so use FIFOCount, which is the element count the spec provides. ModbusPDU01ReadCoilsResponse additionally derived from _ModbusPDUNoPayload, whose extract_padding discards everything after the PDU, so a following ADU was dropped rather than mangled. Its counterpart ModbusPDU02ReadDiscreteInputsResponse already derives from Packet; match it. Reported and diagnosed by Insanitree in #4096. Fixes: #4096 AI-Assisted: yes (Claude Fable 5, via Claude Code) --- scapy/contrib/modbus.py | 14 +++++++------- test/contrib/modbus.uts | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/scapy/contrib/modbus.py b/scapy/contrib/modbus.py index 9e9cd2ed915..8129bf263cf 100644 --- a/scapy/contrib/modbus.py +++ b/scapy/contrib/modbus.py @@ -46,7 +46,7 @@ class ModbusPDU01ReadCoilsRequest(_ModbusPDUNoPayload): XShortField("quantity", 0x0001)] -class ModbusPDU01ReadCoilsResponse(_ModbusPDUNoPayload): +class ModbusPDU01ReadCoilsResponse(Packet): name = "Read Coils Response" fields_desc = [XByteField("funcCode", 0x01), BitFieldLenField("byteCount", None, 8, @@ -102,7 +102,7 @@ class ModbusPDU03ReadHoldingRegistersResponse(Packet): adjust=lambda pkt, x: x * 2), FieldListField("registerVal", [0x0000], ShortField("", 0x0000), - count_from=lambda pkt: pkt.byteCount, + count_from=lambda pkt: pkt.byteCount // 2, max_count=123)] @@ -127,7 +127,7 @@ class ModbusPDU04ReadInputRegistersResponse(Packet): adjust=lambda pkt, x: x * 2), FieldListField("registerVal", [0x0000], ShortField("", 0x0000), - count_from=lambda pkt: pkt.byteCount)] + count_from=lambda pkt: pkt.byteCount // 2)] class ModbusPDU04ReadInputRegistersError(Packet): @@ -313,7 +313,7 @@ class ModbusPDU10WriteMultipleRegistersRequest(Packet): adjust=lambda pkt, x: x * 2), FieldListField("outputsValue", [0x0000], XShortField("", 0x0000), - count_from=lambda pkt: pkt.byteCount)] + count_from=lambda pkt: pkt.byteCount // 2)] class ModbusPDU10WriteMultipleRegistersResponse(Packet): @@ -518,7 +518,7 @@ class ModbusPDU17ReadWriteMultipleRegistersRequest(Packet): adjust=lambda pkt, x: x * 2), FieldListField("writeRegistersValue", [0x0000], XShortField("", 0x0000), - count_from=lambda pkt: pkt.byteCount)] + count_from=lambda pkt: pkt.byteCount // 2)] class ModbusPDU17ReadWriteMultipleRegistersResponse(Packet): @@ -529,7 +529,7 @@ class ModbusPDU17ReadWriteMultipleRegistersResponse(Packet): adjust=lambda pkt, x: x * 2), FieldListField("registerVal", [0x0000], ShortField("", 0x0000), - count_from=lambda pkt: pkt.byteCount)] + count_from=lambda pkt: pkt.byteCount // 2)] class ModbusPDU17ReadWriteMultipleRegistersError(Packet): @@ -552,7 +552,7 @@ class ModbusPDU18ReadFIFOQueueResponse(Packet): adjust=lambda pkt, p: p * 2 + 2), BitFieldLenField("FIFOCount", None, 16, count_of="FIFOVal"), FieldListField("FIFOVal", [], ShortField("", 0x0000), - count_from=lambda pkt: pkt.byteCount)] + count_from=lambda pkt: pkt.FIFOCount)] class ModbusPDU18ReadFIFOQueueError(Packet): diff --git a/test/contrib/modbus.uts b/test/contrib/modbus.uts index ed9f89564b8..a4576557857 100644 --- a/test/contrib/modbus.uts +++ b/test/contrib/modbus.uts @@ -541,3 +541,42 @@ pkt = ModbusPDUUserDefinedFunctionCodeRequest(b'M\x00\x05\x00\n') pkt = next(iter(pkt)) assert pkt.mb_payload == b'\x00\x05\x00\n' + ++ Test back-to-back ADUs in one buffer + += Register arrays are not over-read when a second ADU follows +# GH4096: byteCount is a byte count, but count_from expects an element count, +# so register arrays consumed twice as many items as they should and swallowed +# the header of the following ADU. +adu = raw(ModbusADUResponse(transId=1, unitId=0xff) / ModbusPDU04ReadInputRegistersResponse(registerVal=[0x1111, 0x2222, 0x3333])) +pkt = ModbusADUResponse(adu + adu) +assert pkt[ModbusPDU04ReadInputRegistersResponse].registerVal == [0x1111, 0x2222, 0x3333] +assert raw(pkt[ModbusPDU04ReadInputRegistersResponse].payload) == adu + += Read Coils response keeps the ADU that follows it +# GH4096: ModbusPDU01ReadCoilsResponse derived from _ModbusPDUNoPayload, whose +# extract_padding discards everything after the PDU, so the second ADU was lost. +adu = raw(ModbusADUResponse(transId=1, unitId=0xff) / ModbusPDU01ReadCoilsResponse(coilStatus=[0x11, 0x22, 0x33])) +pkt = ModbusADUResponse(adu + adu) +assert pkt[ModbusPDU01ReadCoilsResponse].coilStatus == [0x11, 0x22, 0x33] +assert raw(pkt[ModbusPDU01ReadCoilsResponse].payload) == adu + += Every byteCount-driven array round-trips across concatenated ADUs +# GH4096: covers each PDU whose variable array is sized from byteCount. +cases = [ + (ModbusADUResponse, ModbusPDU01ReadCoilsResponse, "coilStatus", [0x11, 0x22, 0x33]), + (ModbusADUResponse, ModbusPDU02ReadDiscreteInputsResponse, "inputStatus", [0x11, 0x22, 0x33]), + (ModbusADUResponse, ModbusPDU03ReadHoldingRegistersResponse, "registerVal", [0x1111, 0x2222, 0x3333]), + (ModbusADUResponse, ModbusPDU04ReadInputRegistersResponse, "registerVal", [0x1111, 0x2222, 0x3333]), + (ModbusADURequest, ModbusPDU0FWriteMultipleCoilsRequest, "outputsValue", [0x11, 0x22, 0x33]), + (ModbusADURequest, ModbusPDU10WriteMultipleRegistersRequest, "outputsValue", [0x1111, 0x2222, 0x3333]), + (ModbusADURequest, ModbusPDU17ReadWriteMultipleRegistersRequest, "writeRegistersValue", [0x1111, 0x2222, 0x3333]), + (ModbusADUResponse, ModbusPDU17ReadWriteMultipleRegistersResponse, "registerVal", [0x1111, 0x2222, 0x3333]), + (ModbusADUResponse, ModbusPDU18ReadFIFOQueueResponse, "FIFOVal", [0x1111, 0x2222, 0x3333]), +] +for adu_cls, pdu_cls, field, values in cases: + adu = raw(adu_cls(transId=1, unitId=0xff) / pdu_cls(**{field: values})) + assert list(getattr(adu_cls(adu)[pdu_cls], field)) == values, pdu_cls.__name__ + pkt = adu_cls(adu + adu) + assert list(getattr(pkt[pdu_cls], field)) == values, pdu_cls.__name__ + assert raw(pkt[pdu_cls].payload) == adu, pdu_cls.__name__